Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: separate wallet from node #10973

Merged
merged 4 commits into from Mar 21, 2019
Merged

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Aug 1, 2017

This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract Chain class in src/interfaces/ instead of using global variables like cs_main, chainActive, and g_connman. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the hide-globals script in #10244).

This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:

-    wtx.nTimeReceived = GetAdjustedTime();
+    wtx.nTimeReceived = m_chain->getAdjustedTime();

This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through CValidationInterface, CRPCTable, and CCoinsViewMemPool interfaces) with code that uses the Chain interface.

These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:

  • Provide a single place to describe the interface between wallet and node code.
  • Can make better wallet testing possible, because the Chain object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 2, 2017
This commit just moves a few function declarations and updates callers.
Function bodies are moved in two followup MOVEONLY commits.

This change is desirable because wallet.h/cpp are monolithic and hard to
navigate, so pulling things out and grouping together pieces of related
functionality should improve the organization.

Another proximate motivation is the wallet process separation work in
bitcoin#10973, where (at least initially)
parameter parsing and fee estimation are still done in the main process rather
than the wallet process, and having functions that run in different processes
scrambled up throughout wallet.cpp is unnecessarily confusing.
@jonasschnelli
Copy link
Contributor

Concept ACK

@ryanofsky ryanofsky changed the title WIP: Add IPC layer between node and wallet Add IPC layer between node and wallet Aug 14, 2017
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 14, 2017
This commit just moves a few function declarations and updates callers.
Function bodies are moved in two followup MOVEONLY commits.

This change is desirable because wallet.h/cpp are monolithic and hard to
navigate, so pulling things out and grouping together pieces of related
functionality should improve the organization.

Another proximate motivation is the wallet process separation work in
bitcoin#10973, where (at least initially)
parameter parsing and fee estimation are still done in the main process rather
than the wallet process, and having functions that run in different processes
scrambled up throughout wallet.cpp is unnecessarily confusing.
laanwj added a commit that referenced this pull request Aug 25, 2017
f01103c MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (Russell Yanofsky)
e7fe320 MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (Russell Yanofsky)
d97fe20 Move some static functions out of wallet.h/cpp (Russell Yanofsky)

Pull request description:

  This just moves some static wallet fee and init functions out of `wallet/wallet.cpp` and into new `wallet/fees.cpp` and `wallet/init.cpp` source files. There is one commit updating declarations and callers, followed by two MOVEONLY commits actually moving the function bodies.

  This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization.

  Another motivation is the wallet process separation work in #10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing.

Tree-SHA512: 6e6982ff82b2ab4e681c043907e2b1801ceb9513394730070f16c46ad338278a863f5b3759aa13db76a259b268b1c919c81f4e339f0796a3cfb990161e8c316d
@ryanofsky ryanofsky force-pushed the pr/wipc-sep branch 2 times, most recently from 8e7fdd5 to 8fb011e Compare August 29, 2017 09:54
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Sep 1, 2017

@jnewbery, the change to stop deduping link arguments is in Makefile.am here

Copy link
Contributor

@JeremyRubin JeremyRubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a superficial review of the code & the approach seems reasonable to me!

utACKs from me for the commits checked below...

  • 40afc26 Add skeleton chain and client classes
  • 5269b56 Pass chain and client variables where needed
  • 7bfb409 Remove uses of wallet functions in init.cpp
  • e26e00b Remove uses of cs_main in wallet code
  • 0a3464b Pass chain locked variables where needed
  • c119463 Remove uses of chainActive and mapBlockIndex in wallet code
  • 2dd492b Remove uses of CheckFinalTx in wallet code
  • ebb23cb Remove use of IsWitnessEnabled in wallet code
  • dd180da Remove use of AcceptToMemoryPool in wallet code
  • 83be2e6 Remove uses of GetVirtualTransactionSize in wallet code
  • 8931629 Remove use of IsRBFOptIn in wallet code
  • 684e807 Remove use of GetCountWithDescendants in wallet code
  • f3f36e8 Remove use of g_connman / PushInventory in wallet code
  • c9d049c Remove use of TransactionWithinChainLimit in wallet code
  • 04f6a9e Remove use of CalculateMemPoolAncestors in wallet code
  • c510dde Remove uses of fee globals in wallet code
  • 9849c4d Remove uses of fPruneMode in wallet code
  • ab488c8 Remove uses of g_connman in wallet code
  • 090411a Remove uses of GetAdjustedTime in wallet code
  • 64ad91d Remove uses of InitMessage/Warning/Error in wallet code
  • 3ab3335 Remove use CValidationInterface in wallet code
  • 8756810 Remove use of CRPCTable::appendCommand in wallet code
  • 35bfb7f Remove use of generateBlocks in wallet code
  • 8fb011e Remove uses of ParseConfirmTarget in wallet code

src/ipc/local/bitcoind.cpp Outdated Show resolved Hide resolved
src/ipc/interfaces.h Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/ipc/interfaces.h Outdated Show resolved Hide resolved
@ryanofsky ryanofsky force-pushed the pr/wipc-sep branch 5 times, most recently from e0688ad to 2015123 Compare September 22, 2017 21:48
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 22, 2017
Return unset optional instead of -1 from functions trying to return heights of
blocks that might not exist.

Change suggested by Jeremy Rubin <jeremy.l.rubin@gmail.com>
bitcoin#10973 (comment)
5tefan added a commit to 5tefan/dash that referenced this pull request Jul 28, 2021
Merges bitcoin#14636: Avoid using numeric_limits for sequence
numbers and lock times.

5352030 Avoid using numeric_limits for sequence numbers and lock
            times (Russell Yanofsky)
bafb921 Remove duplicated code (Hennadii Stepanov)
e4dc39b Replace platform dependent type with proper const
            (Hennadii Stepanov)

Pull request description:

  Switches to named constants, because numeric_limits calls can be
harder to read and less portable.

  Change was suggested by jamesob in
bitcoin#10973 (comment)

  There are no changes in behavior except on some platforms we don't
support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and
`MutateTxAddInput` functions would now work correctly.
5tefan added a commit to 5tefan/dash that referenced this pull request Jul 28, 2021
Merges bitcoin#14636: Avoid using numeric_limits for sequence
numbers and lock times.

5352030 Avoid using numeric_limits for sequence numbers and lock
            times (Russell Yanofsky)
bafb921 Remove duplicated code (Hennadii Stepanov)
e4dc39b Replace platform dependent type with proper const
            (Hennadii Stepanov)

Pull request description:

  Switches to named constants, because numeric_limits calls can be
harder to read and less portable.

  Change was suggested by jamesob in
bitcoin#10973 (comment)

  There are no changes in behavior except on some platforms we don't
support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and
`MutateTxAddInput` functions would now work correctly.
PastaPastaPasta pushed a commit to dashpay/dash that referenced this pull request Jul 28, 2021
…and lock times (#4296)

Merges bitcoin#14636: Avoid using numeric_limits for sequence
numbers and lock times.

5352030 Avoid using numeric_limits for sequence numbers and lock
            times (Russell Yanofsky)
bafb921 Remove duplicated code (Hennadii Stepanov)
e4dc39b Replace platform dependent type with proper const
            (Hennadii Stepanov)

Pull request description:

  Switches to named constants, because numeric_limits calls can be
harder to read and less portable.

  Change was suggested by jamesob in
bitcoin#10973 (comment)

  There are no changes in behavior except on some platforms we don't
support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and
`MutateTxAddInput` functions would now work correctly.
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 23, 2021
…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

# Conflicts:
#	src/Makefile.am
#	src/interfaces/wallet.cpp
#	src/qt/test/wallettests.cpp
#	src/wallet/rpcdump.cpp
#	src/wallet/rpcwallet.cpp
#	src/wallet/test/wallet_tests.cpp
#	src/wallet/wallet.cpp
#	src/wallet/wallet.h
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2021
…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

# Conflicts:
#	src/Makefile.am
#	src/interfaces/wallet.cpp
#	src/qt/test/wallettests.cpp
#	src/wallet/rpcdump.cpp
#	src/wallet/rpcwallet.cpp
#	src/wallet/test/wallet_tests.cpp
#	src/wallet/wallet.cpp
#	src/wallet/wallet.h
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 25, 2021
…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

# Conflicts:
#	src/Makefile.am
#	src/interfaces/wallet.cpp
#	src/qt/test/wallettests.cpp
#	src/wallet/rpcdump.cpp
#	src/wallet/rpcwallet.cpp
#	src/wallet/test/wallet_tests.cpp
#	src/wallet/wallet.cpp
#	src/wallet/wallet.h
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 25, 2021
…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

# Conflicts:
#	src/Makefile.am
#	src/interfaces/wallet.cpp
#	src/qt/test/wallettests.cpp
#	src/wallet/rpcdump.cpp
#	src/wallet/rpcwallet.cpp
#	src/wallet/test/wallet_tests.cpp
#	src/wallet/wallet.cpp
#	src/wallet/wallet.h
kwvg added a commit to kwvg/dash that referenced this pull request Oct 31, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 1, 2021
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Nov 1, 2021
…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Nov 1, 2021
…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Nov 3, 2021
…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8
kwvg added a commit to kwvg/dash that referenced this pull request Nov 4, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 4, 2021
malbit pushed a commit to malbit/raptoreum that referenced this pull request Nov 5, 2021
…pp #10976

Move some static functions out of wallet.h/cpp

This commit just moves a few function declarations and updates callers.
Function bodies are moved in two followup MOVEONLY commits.

This change is desirable because wallet.h/cpp are monolithic and hard to
navigate, so pulling things out and grouping together pieces of related
functionality should improve the organization.

Another proximate motivation is the wallet process separation work in
bitcoin/bitcoin#10973, where (at least initially)
parameter parsing and fee estimation are still done in the main process rather
than the wallet process, and having functions that run in different processes
scrambled up throughout wallet.cpp is unnecessarily confusing.

MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp

make it actual move only

Signed-off-by: Pasta <pasta@dashboost.org>

MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp

make it actual move only

Signed-off-by: Pasta <pasta@dashboost.org>

add keepass include

Signed-off-by: Pasta <pasta@dashboost.org>
kwvg added a commit to kwvg/dash that referenced this pull request Nov 6, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 14, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 14, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 14, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 14, 2021
UdjinM6 added a commit to dashpay/dash that referenced this pull request Nov 15, 2021
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet